-
Notifications
You must be signed in to change notification settings - Fork 258
fix(pagination): resolve page number conversion and key-based pagination issues #2003
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a new pagination helper ReadPageRequest in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI command
participant CU as clientutils.ReadPageRequest
participant GRPC as gRPC query handler
User->>CLI: run list command with pagination flags
CLI->>CU: ReadPageRequest(cmd.Flags())
CU-->>CLI: PageRequest or error
CLI->>GRPC: Query with PageRequest
GRPC-->>CLI: Response (items + pagination)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
🧹 Nitpick comments (6)
x/market/client/cli/lease.go (1)
63-66: Misleading help text: “Query order” for a lease getter.Minor polish for UX.
Apply:
- Short: "Query order", + Short: "Query lease",x/market/client/cli/bid.go (1)
63-66: Misleading help text: “Query order” for a bid getter.Apply:
- Short: "Query order", + Short: "Query bid",x/market/keeper/grpc_query.go (4)
33-41: Defensively reject requests that set both Key and Offset.Client fixes help, but gRPC should also enforce mutual exclusivity to protect server invariants.
Apply this diff:
if req.Pagination == nil { req.Pagination = &sdkquery.PageRequest{} } else if req.Pagination != nil && req.Pagination.Offset > 0 && req.Filters.State == "" { return nil, status.Error(codes.InvalidArgument, "invalid request parameters. if offset is set, filter.state must be provided") } +// Defensive: page key and numeric offset are mutually exclusive +if len(req.Pagination.Key) > 0 && req.Pagination.Offset > 0 { + return nil, status.Error(codes.InvalidArgument, "invalid request parameters. set either page key or offset, not both") +}
189-196: Reverse-search decode fix (unsolicited[0]) is correct.This removes the off‑by‑index bug and matches the encoder which sets a single flag byte.
For readability, consider a named constant:
+const unsolicitedReverseSearch byte = 1 @@ - if unsolicited[0] == 1 { + if unsolicited[0] == unsolicitedReverseSearch { reverseSearch = true }
164-172: Add the same key/offset mutual‑exclusion guard for Bids.Mirror the Orders validation to block ambiguous pagination requests.
Apply this diff:
if req.Pagination == nil { req.Pagination = &sdkquery.PageRequest{} } else if req.Pagination != nil && req.Pagination.Offset > 0 && req.Filters.State == "" { return nil, status.Error(codes.InvalidArgument, "invalid request parameters. if offset is set, filter.state must be provided") } +if len(req.Pagination.Key) > 0 && req.Pagination.Offset > 0 { + return nil, status.Error(codes.InvalidArgument, "invalid request parameters. set either page key or offset, not both") +}
318-326: Also enforce key/offset mutual exclusivity for Leases.Keeps all three query handlers consistent and resilient against malformed clients.
Apply this diff:
if req.Pagination == nil { req.Pagination = &sdkquery.PageRequest{} } else if req.Pagination != nil && req.Pagination.Offset > 0 && req.Filters.State == "" { return nil, status.Error(codes.InvalidArgument, "invalid request parameters. if offset is set, filter.state must be provided") } +if len(req.Pagination.Key) > 0 && req.Pagination.Offset > 0 { + return nil, status.Error(codes.InvalidArgument, "invalid request parameters. set either page key or offset, not both") +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/utils.go(1 hunks)x/audit/client/cli/query.go(2 hunks)x/cert/client/cli/query.go(2 hunks)x/deployment/client/cli/query.go(2 hunks)x/market/client/cli/bid.go(2 hunks)x/market/client/cli/lease.go(2 hunks)x/market/client/cli/order.go(2 hunks)x/market/keeper/grpc_query.go(3 hunks)x/provider/client/cli/query.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
x/audit/client/cli/query.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
x/provider/client/cli/query.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
x/market/client/cli/bid.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
x/market/client/cli/lease.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
x/deployment/client/cli/query.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
x/cert/client/cli/query.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
x/market/client/cli/order.go (1)
client/utils.go (1)
ReadPageRequest(13-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: lint
- GitHub Check: build-bins
- GitHub Check: tests
- GitHub Check: coverage
- GitHub Check: release-dry-run
🔇 Additional comments (2)
x/market/keeper/grpc_query.go (2)
276-291: Keeping Bids NextKey raw and encoding with flags at the end is correct.LGTM. The token now round‑trips cleanly across boundary pages and reverse index scans.
128-147: Approve — keep SDK NextKey rawVerified: EncodePaginationKey/DecodePaginationKey are used consistently across grpc_query.go files, util/query/pagination.go provides implementations and tests (util/query/pagination_test.go), and no NextKey+searchPrefix mutations were found. LGTM.
Description
Closes: #XXXX
This PR resolves critical pagination bugs that were preventing proper page navigation in CLI queries and breaking provider startup order discovery. The issues included:
--page Nflags were not converting to proper offset valuesKey Changes:
Client-side fixes:
ReadPageRequestfunction with page-to-offset conversion (--page 2→offset = (2-1) * limit)x/provider/client/cli/query.gox/market/client/cli/{order,bid,lease}.gox/deployment/client/cli/query.gox/cert/client/cli/query.gox/audit/client/cli/query.goServer-side fixes:
unsolicited[1]→unsolicited[0])Testing:
--page Nand--page-keymethods return identical results across all query typesAuthor Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.md